Language Refactor 3 #937
Language Refactor 3 #937
Conversation
github-actions
bot
added
Changes: C#
FoxxoTrystan
requested review from
FoxxoTrystan,
a team,
VMSolidus,
DEATHB4DEFEAT and
Pspritechologist
and removed request for
a team
|
|
||
| /// <summary> | ||
| /// List of languages this entity can speak at the current moment. | ||
| /// </summary> | ||
| public List<string> SpokenLanguages = []; | ||
| [DataField] | ||
| public List<ProtoId<LanguagePrototype>> SpokenLanguages = []; |
There was a problem hiding this comment.
| public List<ProtoId<LanguagePrototype>> SpokenLanguages = []; | |
| public List<ProtoId<LanguagePrototype>>? SpokenLanguages = default!; |
We really should be doing these as Nullable lists and not a default empty list. Anywhere you logically check "If empty return", you can instead check "if null return". This is actually overwhelmingly preferred now according to Microsoft's own documentation, ever since Dotnet 8 added the ability for IDE's to check for null references before compiling. The given reason is that it offers a substantial performance improvement, while still using the same general logic structure.
There was a problem hiding this comment.
I don't really see the benefit, but I see how it would be highly inconvenient for every caller to check those fields for null, when in reality they would never be null. The performance difference should be very negligible here, although if it concerns you a lot, I simply can change the initializer to default!, without changing the field type.
|
Everything works as far as I could test, so marking as ready for review. |
|
And that's why I merge master before testing PRs now. Excellent. This PR doesn't work. :) |
|
merge is unrelated, I broke the linter somehow. |
|
The linter was unhappy I had an empty string as the default value for LSC.CurrentLanguage. Well, I revoked its right to check that field. |
There was a problem hiding this comment.
Can't approve this until you add back in the functionality for removing UniversalLanguageSpeakerComponent also removing the Universal Language. Here I have a Cataloguer that has been mindbroken(which removes the UniversalLanguageSpeakerComponent), but even after the component is removed, the Cataloguer retained their ability to speak Universal.
Mmmm, this just means the xenoglossy system is broken as it does not call UpdateEntityLanguages when adding/removing language-related comps. I'll make it get called automatically on the comp removal event... later, when I get time. |
FoxxoTrystan
added
Priority: 2-High
FoxxoTrystan
added
the
Status: Awaiting Changes
Is this PR Abandoned? |
|
I forgot this PR existed, and no one reminded me weeee-2024-10-07_21.31.04.mp4 |
github-actions
bot
added
Status: Needs Review
Description
This significantly improves the quality of the language system by fixing the mistakes I've made almost a year ago while developing it.
Mainly, this throws away the old half-broken way of networking in favor of the component state system provided by RT. Language speaker comp is now shared with SendOnlyToOwner = true, and its state is handled manually.
In addition to that, this brings the following changes:
Changelog
No cl